[POS Orders] Render sales channel option as order filter #15878
Conversation
# Conflicts: # Modules/Sources/Experiments/DefaultFeatureFlagService.swift
|
|
joshheald
left a comment
joshheald
left a comment
There was a problem hiding this comment.
Works as expected. A few suggestions inline which would make the PR more compact and reduce overhead 😊
| if featureFlagService.isFeatureFlagEnabled(.pointOfSaleOrdersi2) { | ||
| filterTypeViewModels.append(salesChannelFilterViewModel) | ||
| } |
There was a problem hiding this comment.
If this is the only reason the filterTypeViewModels needs to be a var, it would be better to make a local var, and then set the property with it when you're done constructing it.
There was a problem hiding this comment.
It is the only reason indeed, changed on 4f93bca
| static let rowTitleOrderStatus = NSLocalizedString( | ||
| "filterOrderListViewModel.OrderListFilter.rowTitleOrderStatus", | ||
| value: "Order Status", | ||
| comment: "Row title for filtering orders by order status.") | ||
|
|
||
| static let rowTitleDateRange = NSLocalizedString( | ||
| "filterOrderListViewModel.OrderListFilter.rowTitleDateRange", | ||
| value: "Date Range", | ||
| comment: "Row title for filtering orders by date range.") |
There was a problem hiding this comment.
There's no real benefit to changing existing localized string definitions to use the key, value, comment initialiser, AFAIK.
It incurrs cost/work, because the translation will need to be done again in all the langages even though it's not changed, and I don't know if Glotpress is good at linking up the old version with the new version...
There was a problem hiding this comment.
Interesting point, thanks! Since p91TBi-aJl-p2 I've been updating these to adopt the "DNS key thingy" approach every time I had to directly work with them. I'll take that into account moving forward 👍
There was a problem hiding this comment.
Yeah, if you're changing a string, at all, it's definitely right to add the reverse DNS key.
| var description: String { | ||
| switch self { | ||
| case .pointOfSale: | ||
| return "pos" |
There was a problem hiding this comment.
Should this be upper case?
There was a problem hiding this comment.
It can! And most likely it will soon, updated de53799
| let dateRange: OrderDateRangeFilter? | ||
| let product: FilterOrdersByProduct? | ||
| let customer: CustomerFilter? | ||
| let salesChannel: SalesChannelFilter? |
There was a problem hiding this comment.
| let salesChannel: SalesChannelFilter? | |
| let salesChannel: SalesChannelFilter? = nil |
You don't need to change it now, but for a partial PR like this, it might be better to use the default. It saves you a bunch of changes in the tests, which you're probably going to have to change again when you implement it properly. The compiler will still help you catch them all when you remove the default in a future PR.
There was a problem hiding this comment.
True 😅 , thanks for pointing it out.
Closes WOOMOB-708
Closes WOOMOB-709
Closes WOOMOB-710
Description
This PR starts the i2 for POS Orders by rendering a new "Sales Channel" option within the preexisting filters within Order List. This is not functional yet, merely an UI update behind a feature flag.
Screen.Recording.2025-07-08.at.11.37.58.mov
Changes
While I tried to make the minimal changes to only show the UI, I found the current filters quite complicated to follow. Luckily most changes were enforced by the compiler:
OrdersRootViewControllerowns filters, via aFilterOrderListViewModel. This view model holds each individual filter as its own type and associatedFilterTypeViewModel. These are created on demand as we tap the filters button:pointOfSaleOrdersi2feature flagsalesChannelcase toFilterListViewControllerSalesChannelFilter, most likely we'll fetch this from Yosemite/Networking later onFilterOrderListViewModel, we append the new filter only when the feature flag is onWOOMOB-781Testing information
Filters, observe that the new filter appears as an option. Tapping will push an empty view into the stack.pointOfSaleOrdersi2tofalseand re-run the app. The new filter shouldn't be visible.